AGENT-246: Temporarily require at least one NMStateConfig to be specified #6239
Conversation
zaneb
force-pushed
the
nmstate-required
branch
from
5a86e0e to
1a691d7
Compare
We want to get the first static IP address, not the IP address of the first interface.
…fied Numerous places assume that there will be at least one NMStateConfig: * https://bugzilla.redhat.com/show_bug.cgi?id=2116489 * https://bugzilla.redhat.com/show_bug.cgi?id=2115770 * https://bugzilla.redhat.com/show_bug.cgi?id=2115798 * https://bugzilla.redhat.com/show_bug.cgi?id=2115803 * https://bugzilla.redhat.com/show_bug.cgi?id=2117302 Since these are not easily discoverable by users, temporarily reinstate the requirement that at least one NMState configuration be provided (even if the RendezvousIP is specified). This will provide users with an upfront error rather than a difficult-to-debug mystery.
zaneb
force-pushed
the
nmstate-required
branch
from
1a691d7 to
a17dfe3
Compare
| }, | ||
| } | ||
|
|
||
| if len(agentManifests.NMStateConfigs) == 0 { |
There was a problem hiding this comment.
I'd recommend to perform this check in the related asset, here:
There was a problem hiding this comment.
It's legal to e.g. use an empty agent-config to create the cluster-manifests and only fill in the NMStateConfigs after. So we don't want to validate too early, only when you go to create the image and you don't have any NMStateConfigs.
There was a problem hiding this comment.
Should the message be more clear that either provide at least one nmstateconfig in cluster-manifests/nmstateconfig.yaml or if providing agent-config.yaml provide at least one nmstateconfig in the networkConfig?
There was a problem hiding this comment.
That's right, but it's just the NMStateConfig asset that loads the content of the cluster-manifests/nmstateconfig.yaml file if present on the disk. The AgentManifests it's just a collector, and it's resolved as a dependency in stack by the Ignition one, so not sure if I'm missing something but the validation location doesn't seem the right one?
There was a problem hiding this comment.
I'm not sure if the create cluster-manifests it's in good shape now, but if you run it with just the install-config.yaml (and without an agent-config.yaml, so that it will get generated), currently the nil check will already fail
There was a problem hiding this comment.
if the validation is placed in nmstateconfig, then "create cluster-manifests" will immediately fail, thus forcing you to provide the nmstateconfig in the agent-config.
So wouldn't be better in general to provide an agent-config template so that the NMStateConfig will not fail, rather than moving the check into another asset?
There was a problem hiding this comment.
So wouldn't be better in general
This check is going away again in a couple of weeks, as soon as we have fixed all of the linked bugs. Let's not get too general.
to provide an agent-config template so that the NMStateConfig will not fail, rather than moving the check into another asset?
That wouldn't solve the case where you intentionally create an agent-config with no NMState data, with the intent to customise the manifests to add NMState data after create cluster-manifests.
The only way to catch that case is to make sure the validation doesn't run until you do create image, and the only way to do that is to put it in an asset that is only required by the image target.
There was a problem hiding this comment.
Agree, especially considering if that check will go away soon. In general my concerns are about not making an asset self-sufficient, which could indicate a potential problem somewhere else
There was a problem hiding this comment.
Btw, the aforementioned scenario was still broken for me, I will re-test it with the latest changes as a double-check
|
@zaneb: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
| }, | ||
| } | ||
|
|
||
| if len(agentManifests.NMStateConfigs) == 0 { |
There was a problem hiding this comment.
I think what Zane has currently implemented allows a user to first run "create cluster-manifests", edit the nmstateconfig to make any changes, and then run "create image".
If the validation is placed in nmstateconfig, then "create cluster-manifests" will immediately fail, thus forcing you to provide the nmstateconfig in the agent-config.
| }, | ||
| } | ||
|
|
||
| if len(agentManifests.NMStateConfigs) == 0 { |
There was a problem hiding this comment.
Btw, if none of them (agent-config and nmstateconfig) are provided I'd expect that this validation will fail
From my testing, the validation in ignition works.
|
Tested with dev-scripts |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bfournie The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
6 participants
Numerous places assume that there will be at least one NMStateConfig:
Since these are not easily discoverable by users, temporarily reinstate
the requirement that at least one NMState configuration be provided
(even if the RendezvousIP is specified). This will provide users with an
upfront error rather than a difficult-to-debug mystery.